-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proto: Port *.proto to proto3 #2
Conversation
Define syntax to fix [1]: protoc --go_out=./go config.proto [libprotobuf WARNING google/protobuf/compiler/parser.cc:492] No syntax specified for the proto file. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.) Drop 'optional' (with 'sed -i 's/\toptional /\t/' *.proto') to fix: protoc --go_out=./go config.proto config.proto:9:18: Explicit 'optional' labels are disallowed in the Proto3 syntax. To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default. Replace the User extensions with 'Any' [2,3] to fix: protoc --go_out=./go config.proto config.proto: Extensions in proto3 are only allowed for defining options. Drop required (with 'sed -i 's/\trequired /\t/' *.proto') to fix: protoc --go_out=./go runtime_config.proto runtime_config.proto: Required fields are not allowed in proto3. Drop DefaultState to fix: protoc --go_out=./go runtime_config.proto runtime_config.proto: Explicit default values are not allowed in proto3. There's still some trouble with the resulting Go: go run ./example.go go/config.pb.go:26:8: cannot find package "google/protobuf" in any of: /usr/lib/go/src/google/protobuf (from $GOROOT) /home/wking/.local/lib/go/src/google/protobuf (from $GOPATH) Makefile:31: recipe for target 'example' failed But I haven't been able to figure that out yet. [1]: https://developers.google.com/protocol-buffers/docs/proto3#simple [2]: https://developers.google.com/protocol-buffers/docs/proto3#any [3]: protocolbuffers/protobuf#828 Signed-off-by: W. Trevor King <wking@tremily.us>
In a separate branch where I tried to handle Any in Go, I got [1]: go run ./example.go go/config.pb.go:26:8: cannot find package "google/protobuf" in any of: /usr/lib/go/src/google/protobuf (from $GOROOT) /home/wking/.local/lib/go/src/google/protobuf (from $GOPATH) Makefile:31: recipe for target 'example' failed This commit switches the main example to C++, because it's protobuf's implementation language, so new features like Any support tend to land there first. The example tries to round-trip source JSON into a protobuf message and back. It does that by reading from config.json (I've skipped runtime.json for now) into a message, and then writing JSON serialized from that message to stdout. Attributes in the input JSON file that aren't represented in the protobuf message structure seem to be silently dropped (which makes forward compatibility somewhat easier, but explicit validation somewhat harder ;). The docs for this sort of thing seem sparse (although there is a stale C++ Any example here [2]). The source skeleton [3] and Makefile rule [4] are based on the protobuf examples. kTypeUrlPrefix [5], GetTypeUrl [6], the resolver handling [7,8], and the basic to/from JSON conversion [9]. The commented-out LinuxUser section was how I figured out what to put in the process.user block of config.json. The bits of that that weren't in [10], I figured out just by reading proto/cpp/config.pb.h. The Makefile rule should likely be using: $(filter %.cc, $(CPP_FILES)) but my more restrictive filter removes runtime_config.pb.cc to avoid: $ make example_cpp c++ example.cc ./cpp/config.pb.cc ./cpp/runtime_config.pb.cc -o example_cpp $(pkg-config --cflags --libs protobuf) -I ./cpp ./cpp/runtime_config.pb.h:647:30: error: expected unqualified-id before numeric constant const ::oci::LinuxRuntime& linux() const; ^ ... [1]: vbatts#2 [2]: https://developers.google.com/protocol-buffers/docs/proto3#any Although it recommends: status.add_details()->PackFrom(details); And the actual usage would be: status.mutable_details()->PackFrom(details); See [10]. [3]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/examples/add_person.cc [4]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/examples/Makefile#L24-L26 [5]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/util/json_util_test.cc#L52 [6]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/util/json_util_test.cc#L54-L56 [7]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/util/json_util_test.cc#L66-L67 [8]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/util/json_util_test.cc#L85 [9]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/util/json_util_test.cc#L70-L83 [10]: https://github.com/google/protobuf/blob/v3.0.0-beta-1/src/google/protobuf/any_test.cc#L42 Signed-off-by: W. Trevor King <wking@tremily.us>
Using jsonpb [1,2]. Drop the proto.String bits to avoid errors like: ./example.go:17: cannot use proto.String("linux") (type *string) as type string in field value And use a reference to s to avoid: ./example.go:29: cannot use s (type oci.LinuxSpec) as type proto.Message in argument to marshaler.Marshal: oci.LinuxSpec does not implement proto.Message (ProtoMessage method has pointer receiver) [1]: golang/protobuf#44 [2]: http://godoc.org/github.com/golang/protobuf/jsonpb Signed-off-by: W. Trevor King <wking@tremily.us>
To avoid: go run ./example.go go/config.pb.go:26:8: cannot find package "google/protobuf" in any of: /usr/lib/go/src/google/protobuf (from $GOROOT) /home/wking/.local/lib/go/src/google/protobuf (from $GOPATH) Makefile:31: recipe for target 'example' failed The Dockerfile changes will compile Google's any.proto to Go and install it in the GOPATH. Unfortunately, the jsonpb rendering isn't quite right, since it's spitting out: "user": { "type_url": "oci.LinuxUser", "value": "qAYBsAYBuAYFuAYG" }, When it should be spitting out [1]: "user": { "@type": "type.googleapis.com/oci.LinuxUser", "uid": 1, "gid": 1, "additionalGids": [ 5, 6 ] }, [1]: https://developers.google.com/protocol-buffers/docs/proto3#json Signed-off-by: W. Trevor King <wking@tremily.us>
I've figured out the ‘cannot find package "google/protobuf"’ error, and pushed da7a33f, which gives output like:
It looks like golang/protobuf/jsonpb needs some more work to give us the expected:
we see with the C++ example in #3. |
As an alternative to my Dockerfile changes in da7a33f, we could also use go.pedge.io/google-protobuf (see golang/protobuf#50). I don't care either way. |
@wking Note I'm trying to push to get go.pedge.io/google-protobuf just merged into golang/protobuf, I also have go.pedge.io/googleapis now, it's no fun to maintain, but I promise neither will go anywhere (and the go.pedge.io server is dead simple). Of note, I also have https://github.com/peter-edge/go-tools/tree/master/protoc-all (go get go.pedge.io/tools/protoc-all), which I'm happy to walk anyone through, I'm going to rewrite it soon as it's become a bit of a mess but it takes care of all the import paths (and uses go.pedge.io/google-protobuf). |
// Hostname is the container's host name. | ||
optional string hostname = 5; | ||
string hostname = 5; | ||
// Mounts profile configuration for adding mounts to the container's | ||
// filesystem. | ||
repeated MountPoint mounts = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if google is moving away from this, but the general protobuf standard has been to not use plural names for repeated fields, so this would be mount or mount_point.
Just made a few comments with some thoughts/suggestions, no worries but hoping they may be helpful :) |
It became the default with golang/protobuf@9ebc6c4e (2015-10-19) and was removed completely with golang/protobuf@8a5d8e8b (jsonpb: Remove Marshaler.EnumsAsString, 2015-10-21). Signed-off-by: W. Trevor King <wking@tremily.us>
On Sat, Oct 24, 2015 at 10:44:59AM -0700, Peter Edge wrote:
They sound reasonable to me, but I'm trying to keep this branch as |
This is obsolete now. 😅 |
Define syntax to fix:
Drop
optional
(withsed -i 's/\toptional /\t/' *.proto
) to fix:Replace the
User
extensions withAny
(see also,protocolbuffers/protobuf#828) to fix:
Drop
required
(withsed -i 's/\trequired /\t/' *.proto
) to fix:Drop
DefaultState
to fix:There's still some trouble with the resulting Go:
But I haven't been able to figure that out yet.